Conversation
|
|
||
| from .const import DOMAIN | ||
|
|
||
| type AidotConfigEntry = ConfigEntry[AidotDeviceManagerCoordinator] |
There was a problem hiding this comment.
The type alias definition using the type keyword (line 27) requires Python 3.12+. Home Assistant typically needs to support Python 3.11+, so this should use TypeAlias from typing instead: AidotConfigEntry: TypeAlias = ConfigEntry[AidotDeviceManagerCoordinator]
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| def mock_setup_entry() -> Generator[AsyncMock]: | ||
| """Override async_setup_entry.""" | ||
| with patch( | ||
| "homeassistant.components.aidot.async_setup_entry", return_value=True | ||
| ) as mock_setup_entry: | ||
| yield mock_setup_entry |
There was a problem hiding this comment.
The type hint for the mock_setup_entry fixture is incorrect. The fixture yields a MagicMock (from patch()), not an AsyncMock. The return type should be Generator[MagicMock, None, None].
| if added_device_ids: | ||
| async_add_entities( | ||
| AidotLight(hass, coordinator.device_coordinators[device_id]) | ||
| for device_id in added_device_ids | ||
| ) | ||
| lists_added |= added_device_ids | ||
| elif lists_added - new_lists: | ||
| removed_device_ids = lists_added - new_lists | ||
| for device_id in removed_device_ids: | ||
| entity_registry = er.async_get(hass) | ||
| if entity := entity_registry.async_get_entity_id( | ||
| "light", DOMAIN, device_id | ||
| ): | ||
| entity_registry.async_remove(entity) | ||
| lists_added = lists_added - removed_device_ids |
There was a problem hiding this comment.
The add_entities callback uses elif (line 51) to check for removed devices, but it should use a separate if statement. Currently, if devices are both added and removed in the same update, only the added devices will be processed and the removed devices will be ignored. This should be an independent if statement, not elif.
| async def async_turn_on(self, **kwargs: Any) -> None: | ||
| """Fix brightness state synchronization by updating the coordinator's `dimming` field.""" | ||
| if ATTR_BRIGHTNESS in kwargs: | ||
| brightness = kwargs.get(ATTR_BRIGHTNESS, 255) | ||
| await self.coordinator.device_client.async_set_brightness(brightness) | ||
| self.coordinator.data.dimming = brightness | ||
| elif ATTR_COLOR_TEMP_KELVIN in kwargs: | ||
| color_temp_kelvin = kwargs.get(ATTR_COLOR_TEMP_KELVIN) | ||
| await self.coordinator.device_client.async_set_cct(color_temp_kelvin) | ||
| self.coordinator.data.cct = color_temp_kelvin | ||
| self._attr_color_mode = ColorMode.COLOR_TEMP | ||
| elif ATTR_RGBW_COLOR in kwargs: | ||
| rgbw_color = kwargs.get(ATTR_RGBW_COLOR) | ||
| await self.coordinator.device_client.async_set_rgbw(rgbw_color) | ||
| self.coordinator.data.rgbw = rgbw_color | ||
| self._attr_color_mode = ColorMode.RGBW | ||
| else: | ||
| await self.coordinator.device_client.async_turn_on() | ||
|
|
||
| self.coordinator.data.on = True | ||
| self._attr_is_on = True |
There was a problem hiding this comment.
The async_turn_on and async_turn_off methods directly modify self.coordinator.data attributes instead of using the coordinator's async_set_updated_data method. This bypasses the coordinator's update mechanism and can lead to state synchronization issues. Consider using async_set_updated_data to update the data properly.
| delete_lists = self.previous_lists - ( | ||
| current_lists := {device[CONF_ID] for device in filter_device_list} | ||
| ) |
There was a problem hiding this comment.
The code accesses device[CONF_ID] on line 116 without checking if the key exists, but uses device.get(CONF_ID) on line 127. This is inconsistent. If a device lacks CONF_ID, line 116 will raise a KeyError. Either use consistent error handling or verify that CONF_ID is always present in devices returned from the API.
| device | ||
| for device in data[CONF_DEVICE_LIST] | ||
| if ( | ||
| device[CONF_TYPE] == "light" |
There was a problem hiding this comment.
Line 109 accesses device[CONF_TYPE] without checking if the key exists, potentially raising a KeyError. Consider using device.get(CONF_TYPE) with a default value or adding validation to ensure devices returned from async_get_all_device() always contain required fields.
| device[CONF_TYPE] == "light" | |
| device.get(CONF_TYPE) == "light" |
| await self.async_set_unique_id(client.get_identifier()) | ||
| self._abort_if_unique_id_configured() |
There was a problem hiding this comment.
So we have something more specific like a user ID? Do we get a token back that we can get the user ID out of?
| @callback | ||
| def add_entities() -> None: | ||
| """Add light entities.""" | ||
| nonlocal lists_added | ||
| new_lists = { | ||
| device_coordinator.device_client.device_id | ||
| for device_coordinator in coordinator.device_coordinators.values() | ||
| } | ||
|
|
||
| if new_lists - lists_added: | ||
| async_add_entities( | ||
| AidotLight(hass, coordinator.device_coordinators[device_id]) |
There was a problem hiding this comment.
I think that's a nice feature, we even consider that a good practice, but I also think that adding this in this PR will add an extra review burden, so if we can remove it from this PR and add it in a later PR, we can speed up the review
| @@ -0,0 +1,11 @@ | |||
| { | |||
| "domain": "aidot", | |||
| "name": "AiDot Lights Local", | |||
There was a problem hiding this comment.
How does that work, can you elaborate on that?
But when there's no integration already, we just call it the name of the brand, not appending Local. And unless the company is called AiDot Lights, I would just go with AiDor
… light setup, refactor device list to dict
| manufacturer = model_id.split(".")[0] | ||
| model = model_id[len(manufacturer) + 1 :] |
There was a problem hiding this comment.
The model_id parsing assumes the model_id always contains at least one dot. If a device's model_id doesn't match the expected format (e.g., "aidot.light.rgbw"), the extracted model name could be empty or incorrect. Consider adding validation to handle edge cases or use a more robust parsing method like rsplit with maxsplit parameter.
| manufacturer = model_id.split(".")[0] | |
| model = model_id[len(manufacturer) + 1 :] | |
| if "." in model_id: | |
| manufacturer, model = model_id.rsplit(".", 1) | |
| else: | |
| manufacturer = model_id | |
| model = model_id |
Breaking change
Proposed change
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: